Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add support to register plugins by module path #373

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add support to register plugins by module path #373

wants to merge 4 commits into from

Conversation

hannesdelbeke
Copy link
Contributor

@hannesdelbeke hannesdelbeke commented Oct 8, 2021 •
edited
Loading

TLDR: this PR adds support to register a plugin with the path to a python file(module). allowing you to register individual files without having to change your folder structure.

Description:

atm pyblish only supports registering a plugin either

  • by registering the plugin class instance
  • or registering the folder containing the python files (modules) which contain the plugins

the paths are also saved in registered paths, and are returned by

  • registered_paths
  • _registered_paths
  • plugin_paths()

i chose to reuse this variable since it requires minimum changes to the codebase, and is a lot cleaner than having a second var.
it also reads nice, registered paths contains all registered paths. both file and folder.

this was tested with pyblish QML and lite.

regarding potentially breaking old pipelines/ pyblish setups.
since before a file path was not allowed in registered paths. no one will have any workflows with files in the registered plugin paths.
this leads to my assumption it will be backwards compatible.

the main function using plugin_paths in the pyblish API is discover. which now handles filepaths correctly.
the CLI does not require changes i believe.

incase there is a test that checks if registered paths does not contain a filepath, this test will now fail.

@hannesdelbeke hannesdelbeke force-pushed the feature/register-plugin-modules branch from 9c9f46f to 36d9902 Compare October 8, 2021 13:41
@hannesdelbeke
Copy link
Contributor Author

just discovered the dev docs so PR likely will need some work
https://pyblish.gitbooks.io/developer-guide/content/implementing_a_feature.html

@mottosso
Copy link
Member

I've thought about this and can see the use for it.

However, the discover function is at the root of all of Pyblish, and will need tests to account for all possible cases. All of them. On top of that, this change introduces a lot of new complexity to this function which means a ton of room for error. I'm onboard with the idea, but the implementation is currently too complex. I can take a closer look to see whether there is a simpler way of achieving it, but it may take a while. If on the other hand you can think of an alternative, simpler approach then that would be great.

@hannesdelbeke
Copy link
Contributor Author

fair enough, I'm aware discover is a core func and backwards compatibility is key.

regarding complexity of this PR:
if you look at the PR commits instead of at all changed code, it's easier to follow what's going on

the first 2 commits touch a lot of code, but do not change ANY functionality. just a refactor to avoid duplicate code.
they move code to helper functions, to remove duplicate code.

this is in preparation for the 3rd commit which introduces the actual new functionality:
36d9902
when only looking at this commit is the implementation still to complex?

@hannesdelbeke
Copy link
Contributor Author

hannesdelbeke commented Oct 14, 2021 •
edited
Loading

would it be worth splitting this PR so we can tackle the changes separately?

commit 1 removes duplicate code
pyblish has 2 functions with the same code.
this commit moves the code to a helper function. no changes in any functionality

commit 2 move the code for discovering a module to it's own function, so it's not embedded in the discover function.

notice it does introduce a small change!
instead of passing path, fname, and then later construct abspath
we now pass abspath, and later construct path, fname
this is likely the place where anything would break if it breaks.

commit 3 is where we add the new functionallity.
what happens here is that registered paths now also contains the paths of .py files, instead of folders only.
I can't think of anything where it would break backwards comp by introducing this change though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hannesdelbeke @mottosso